test: add scheduled-to-bound coverage for TestPickBindingsToRoll - #847
Open
Akshita kumari (akshita317) wants to merge 1 commit into
Open
Conversation
TestPickBindingsToRoll carries a `// TODO: add more tests`. The existing
cases cover the main happy paths but leave the scheduled-to-bound
transition thinly tested, and one branch of calculateRealTarget entirely
untested.
Add five table-driven cases:
- scheduled binding whose snapshot is already the latest, which the
existing cases never exercise since they all start from an outdated
snapshot
- PickFixed placement type, which no test in this package covered, so
the ClusterNames branch of calculateRealTarget was never taken
- multiple scheduled bindings with maxSurge zero and nothing ready yet
- a scheduled binding held back by maxSurge zero once ready bound
bindings already meet the target
- multiple scheduled bindings under a percentage maxSurge, pinning the
round-up behaviour of the surge budget
Expected values were derived from the controller logic rather than from
observed output. The maxSurge zero pair is deliberately split: scheduled
bindings are not counted in canBeReadyBindings, so maxSurge zero only
holds a rollout back once ready bindings already meet the target, and the
two cases document that distinction.
calculateRealTarget coverage goes from 70.0% to 80.0%. The remaining
functions were already fully covered, so the other cases add behavioural
assertions rather than new lines.
Signed-off-by: Akshita <110122283+akshita317@users.noreply.github.com>
Contributor
Author
|
CI note for reviewers:
Happy to rebase once that settles if you would rather see a green run before reviewing. |
Member
|
Wei Weng (@weng271190436) can you please review given original issue was opened by you. Thanks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
TestPickBindingsToRollcarries a// TODO: add more tests. This adds five table-driven cases for the scheduled-to-bound transition, which the existing cases cover only thinly, and which left one branch ofcalculateRealTargetuntested entirely.Cases added:
PickFixedPlacementTypeappeared zero times in this package's tests, so theClusterNamesbranch ofcalculateRealTargetwas never takenmaxSurge: 0, nothing readymaxSurge: 0, target already met by ready bound bindingsmaxSurgeThe
maxSurge: 0pair is deliberately split into two cases. Scheduled bindings are not added tocanBeReadyBindings(controller.go:421-431), socalculateMaxToAddreturns the full target number when nothing is ready yet —maxSurge: 0only holds a rollout back once ready bindings already meet the target. The two cases document that distinction, which I found surprising when reading the code.Coverage
The PickFixed case is the one that moves the needle.
pickBindingsToRoll(98.7%),determineBindingsToUpdate(100%) andcalculateMaxToAdd(100%) were already covered, so the remaining four cases add behavioural assertions — particularly the percentage rounding — rather than new lines. I would rather state that plainly than imply more than the numbers support.Expected values were derived from reading
pickBindingsToRoll,determineBindingsToUpdate,calculateMaxToAddandcalculateRealTargetbefore running anything, not adjusted afterwards to match observed output.Testing
All five new cases pass. All other unit tests in the package pass.
One pre-existing failure, unrelated to this change:
This reproduces identically on unmodified
main(I stashed this change and re-ran to confirm), and the CI run fore160c1cd, the commit this branch is based on, is also red. The case builds bindings withtime.Now()-relative transition times, so it looks timing-sensitive rather than caused by any recent change. Flagging it here so it is not misattributed to this PR — happy to open a separate issue if that would be useful.TestAPIswas not run locally as it needs an envtest control plane.Related
Part of #676. That issue also lists a set of untested paths in
pickBindingsToRollitself —IsBindingDiffReported, deletion timestamps on scheduled bindings, negativemaxNumberToRemove, and others. Those are a separate concern from the scheduled-to-bound group and are left for follow-up rather than bundled here, so the// TODOcomment stays in place.